-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OQS security response process #124
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Spencer Wilson <[email protected]>
Signed-off-by: Spencer Wilson <[email protected]>
Signed-off-by: Spencer Wilson <[email protected]>
ffad71c
to
746fb6e
Compare
security/response-process.md
Outdated
|
||
COMMENT: | ||
We should confirm that all of the members of the VMT | ||
(1) receive some sort of notification when a GitHub security advisory is filed on any OQS repo and (2) have the necessary permissions to view security advisories. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"any OQS repo" -- No. Suggest to agree on a subset. Ideally only those with a diverse, active contributor base which can play ball, err, handle problems (to stay in the picture, there should be more linebackers for that project than quarterbacks)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We haven't enabled vulnerability reporting on all OQS repositories. So really this can be turned into a point about only turning on security vulnerability reporting on repositories that we have a sufficient base to support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I meant (and probably should have clarified) was that we should make sure we're aware of all vulnerability reports, whichever repo they're filed on. For instance, if we have security advisories enabled for oqs-demos, it's possible we'll receive a report there that is pertinent to liboqs, even though (I imagine) we won't offer any sort of security policy for the demos.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, now I get it. Honestly I wasn't aware that security advisories are enabled for all, even ancillary, sub projects... Out of curiosity: When was that decided and how does this fit to this statement
we won't offer any sort of security policy for the demos.
? I would agree to this statement, but then I don't understand this statement now:
it's possible we'll receive a report there that is pertinent to liboqs
AFAIK we didn't enable reporting security issues for demos, right? Just the publication of advisories (which is a weird combination, honestly).
I think this ties to #2 (comment): What is it that @dstebila labels "fully supported" ? Having a security policy and everything as per this conversation that goes with it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly I wasn't aware that security advisories are enabled for all, even ancillary, sub projects... Out of curiosity: When was that decided and how does this fit to this statement
Looking in the Settings tab on Github for oqs-demos, I don't see any option to enable/disable security advisories -- perhaps it's always enabled?
I think this ties to #2 (comment): What is it that @dstebila labels "fully supported" ? Having a security policy and everything as per this conversation that goes with it?
The criteria you stated at the top of #2 defined "Fully supported (at least 2 fully committed maintainers)" which is what I applied. If you want to add having a security policy to that, or other things, I'm open to that, but that probably belongs in the discussion on #2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the latest version, I have proposed limiting the scope of this document to liboqs, oqs-provider, and the language wrappers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I'm happy with this as this basically puts two people as "single failure points" into the line of fire: @vsoftco and @baentsch . My proposal thus is to add oqs-provider and language wrappers to this list only after they have more than 1 achieved (read: regularly active, solving real problems, doing code, i.e., really able to resolve security issues), long-term committed contributor(s). Professionally, I'd argue for at least 3, but as long as we resolve open-quantum-safe/liboqs#2065 before this is merged, "more than 1" should do as the expectations would then be properly set.
For oqsprovider
I could see @bhess as a possible candidate but wouldn't want to "volunteer" him to this commitment (to handle any and all oqsprovider
security incidents within short notice). For language wrappers, the question goes to @vsoftco; What's your take? Who could alleviate you of the pressure to be the only person with "deep knowledge"? Or is this not as much a problem for you as it is for me (oqsprovider
has a line count about an order of magnitude higher than any language wrapper and equally more integration (and hence, possible security failure) points).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about refining the scope as follows:
- The response process only applies to security issues with liboqs itself.
- The VMT will triage reports received on provider or the language wrappers, but if a security issue is found to lie in provider or language wrapper "glue" code, then the report is considered "Out of scope."
- Security releases and announcements for liboqs should still flow down to provider and the language wrappers via corresponding releases/announcements. The work for this should be minimal, and could be done by anybody on the VMT.
My rationale for (2) is that a reporter may file a liboqs-related security issue on provider or a language wrapper if that's where they observe the bug and they don't have the expertise to identify it as a liboqs issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be an OK approach for me and one pretty workable AFAIK for language wrappers: For those, I'd agree that there's a very low probability to find the root cause for security problems there. oqsprovider
is a substantially different beast: I know that PQCA considers it a wrapper or "glue code" (to openssl
) -- but while that used to be true at the beginning this is covering (way) less than half the story by now. Just look at all of the hybrid and composite logic that is true crypto code (creating and manipulating keys and other crypto material) or all the problems created by (standard algs') key material representation or all of the "data juggling" required to satisfy the OpenSSL APIs: As such oqsprovider
certainly does contain security weaknesses "of its own" and thus will be subject to "original" security problem reports, i.e., ones not affecting liboqs
or any upstream algorithm.
That said and thinking some more about it, I actually have the hunch that oqsprovider
contains more code subject to security vulnerabilities than liboqs
if discarding the "upstream" core algorithm logic from consideration and assuming a libcrypto
(OQS_USE_OPENSSL) based build of liboqs
. Created #138 as a possible way to move this "hunch" more towards "knowledge" (and address a pretty general weakness I see in OQS, code coverage testing). Thinking some more about it, I actually wonder whether resolution of #138 shouldn't be a prerequisite for this PR to land?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be an OK approach for me and one pretty workable AFAIK for language wrappers: For those, I'd agree that there's a very low probability to find the root cause for security problems there.
oqsprovider
is a substantially different beast: I know that PQCA considers it a wrapper or "glue code" (toopenssl
) -- but while that used to be true at the beginning this is covering (way) less than half the story by now. Just look at all of the hybrid and composite logic that is true crypto code (creating and manipulating keys and other crypto material) or all the problems created by (standard algs') key material representation or all of the "data juggling" required to satisfy the OpenSSL APIs: As suchoqsprovider
certainly does contain security weaknesses "of its own" and thus will be subject to "original" security problem reports, i.e., ones not affectingliboqs
or any upstream algorithm.That said and thinking some more about it, I actually have the hunch that
oqsprovider
contains more code subject to security vulnerabilities thanliboqs
if discarding the "upstream" core algorithm logic from consideration and assuming alibcrypto
(OQS_USE_OPENSSL) based build ofliboqs
.
I agree with this assessment, and I think it's backed up by the distribution of security reports we've received (which I'll admit is a very small sample).
Created #138 as a possible way to move this "hunch" more towards "knowledge" (and address a pretty general weakness I see in OQS, code coverage testing). Thinking some more about it, I actually wonder whether resolution of #138 shouldn't be a prerequisite for this PR to land?
I don't think it's a prerequisite, but I have put up open-quantum-safe/liboqs#2072 as a first step toward resolving #138.
Co-authored-by: Douglas Stebila <[email protected]> Signed-off-by: Spencer Wilson <[email protected]>
Signed-off-by: Spencer Wilson <[email protected]>
security/response-process.md
Outdated
| Bug | Let the reporter know this is unwanted behavior but not a security issue, and ask them to refile this as a bug. Close the security issue. | | ||
| Feature request | Let the reporter know this is the intended behavior. If they think this behavior could be improved, they can file a feature request. Close the security issue. | | ||
| Vulnerability | Let the reporter know that you have confirmed this unwanted behavior creates a security issue. Proceed with the process. | | ||
| Out of scope | Let the reporter know that the issue lies in upstream code packaged by OQS. Assist them in refiling the report upstream. See the following section for further actions. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there other scenarios that would be considered out of scope? For instance, could a vulnerability outside the OQS threat model be classified as such?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! I've expanded on "out of scope."
Spencer confirmed the report's findings and responded to the reporters on GitHub on 6 November. | ||
The vulnerability was present in upstream code (https://pqc-hqc.org) and pulled into the library via PQClean. | ||
Spencer notified the "main" and "backup" contacts listed on the upstream source's website after coordinating with the reporters. | ||
The only subproject directly affected (by including vulnerable code) was liboqs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this report is closed and dealt with, but re-reading it makes me wonder: If liboqs
is affected, wouldn't that imply that at least the language wrappers are also affected automatically? Also oqsprovider
makes available HQC code if I'm not mistaken. So in case of a more severe problem with liboqs
what would such problem mean for dependent OQS sub projects? Should that be spelled out somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've fleshed out the main document to specify that oqs-provider and the language wrappers should also have advisories published whenever a liboqs advisory is published.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK for me, if the understanding is that such advisories can/will be done by anyone in the VMT and not fall as responsibility to the committer/maintainers of the sub projects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refinement addressing this now suggested here: #124 (comment)
security/response-process.md
Outdated
|
||
#### Determining affected subprojects | ||
|
||
Part of the assessment should involve determining which subprojects are affected by the issue and how to fix the vulnerability throughout the OQS suite. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a follow-up to my previous comment: Are all sub projects to be checked or are there some we don't consider (oqs-demos? ci-containers? profiling?). I'd guess that should also be spelled out ahead of time so the responsible committers and maintainers can agree on their involvement (level). And if indeed a vulnerability should be fixed "througout the OQS suite", all steps to do so should be clear -- either in writing for manual execution or as an automation/script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a follow-up to my previous comment: Are all sub projects to be checked or are there some we don't consider (oqs-demos? ci-containers? profiling?). I'd guess that should also be spelled out ahead of time so the responsible committers and maintainers can agree on their involvement (level). And if indeed a vulnerability should be fixed "througout the OQS suite", all steps to do so should be clear -- either in writing for manual execution or as an automation/script.
I've refined this in the latest version of the proposal. Input welcome :)
|
||
Development on the private fork proceeds more or less as on public OQS GitHub repos, with one notable exception: | ||
Due to current limitations on GitHub security advisories, the private fork will not have access to OQS CI. | ||
Therefore, thorough testing of the patch should be performed offline. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. This calls for a script to do that, e.g., executing all CI commands locally (and for all sub projects deemed affected): TBD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure executing all CI commands locally is required, as all CI will run after merge to main (i.e., before release). I'm thinking about Windows / Intel macOS / etc jobs that may be difficult to execute without access to GitHub hardware. But, I agree that we should run as many as reasonably possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what wording do you suggest @SWilson4 ?
My proposal is "The patch has to pass all CI tests in local execution including a new test demonstrating absence of the faulty behaviour fixed by the patch."
Signed-off-by: Spencer Wilson <[email protected]>
Signed-off-by: Spencer Wilson <[email protected]>
Signed-off-by: Spencer Wilson <[email protected]>
Signed-off-by: Spencer Wilson <[email protected]>
I think this can be marked as "Ready for review" and proceed with getting final reviews towards merging. |
* Add security announcement mailing list Signed-off-by: Spencer Wilson <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be addressed how members get added to the VMT? And is there a succession plan?
Signed-off-by: Spencer Wilson <[email protected]>
Good point. In the latest commit I've proposed that members are added/removed via TSC vote, with provisions for voluntary self-removal or absence up to one year. |
This PR proposes a security response process for OQS. Please read it over and provide feedback. I expect it will undergo a fair bit of revision, but hopefully this gives us a starting point.
I have left a number of comments requesting feedback on specific points throughout the document. These are clearly marked by COMMENT. I'll keep the PR in draft until all of these are removed.
Closes #60.